[Lens] give data plugin control of filter extraction, injection, and migrations #120305

drewdaemon · 2021-12-02T22:23:02Z

Summary

use data plugin's extract/inject functions instead of our own local stuff
apply filter migrations to filters within Lens visualizations (both standalone and embedded)
introduces a custom definition of lens equality

Question for core team: I duplicated the mergeMigrationFunctionMaps function to make it work for a similar type SavedObjectMigrationMap which passes an addition context argument. See the duplicate function here. Any thoughts on how/if to unify these?

How to Test

Saved Object Migrations

Add the following dummy filter migration here

export const getAllMigrations = (): MigrateFunctionsObject => {
  return {
    '8.1.0': (filters: Filter[]) => filters.map((filter) => ({ ...filter, migrated: true })),
  };
};

Start and open Kibana
Visit "Stack Management" -> "Saved objects"
Import these saved objects
this will add three objects to verify (plus the index pattern)
- Dashboard with Embedded Lens Visualizations with Filters
- standalone table with filters
- standalone bar chart with filter
Verify that each filter in the visualization states
1. Has no indexRefName property. Instead, it should have an index property.
2. Has a migrated: true property.

"Unsaved changes" Modal

Setup

create new lens visualization
save outside of a dashboard
refresh the page. You should still be on the Lens editor.

Does NOT prompt if nothing has changed

Click "Visualize Library" in the breadcrumbs
Expected No "Unsaved changes" warning is shown

DOES prompt if something has changed

Go back to your visualization
Drag a new field to your workspace.
Click "Visualize Library" in the breadcrumbs
Expected: You should see "Unsaved changes" warning

Special case: datasource change

Refresh the page
Set the visualization type to "Metric" on "Record Count" (this setup is necessary to test the datasource equality check in isolation. Using a "Metric" visualization ensures that nothing in the visualization state will change when we change the datasource's data view).
Save visualization
Change the data view used by the datasource

Click "Visualize Library" in the breadcrumbs
Expected: You should see "Unsaved changes" warning

Checklist

Unit or functional tests were updated or added to match the most common scenarios

Risk Matrix

Risk	Probability	Severity	Mitigation/Notes
There are currently no filter migrations defined, so we are building for a future scenario.	Medium	Medium	Unit tests cover applying filter migrations within a Lens visualization. In addition, we will manually test by adding a dummy filter migration and ensuring it is applied.
Difference between Lens `extractFilterReferences` and Data plugin's `extract`. Lens removes `filter.meta.value` since it wasn't persistable at some point.	Low	Medium	A manual test of saving a Lens visualization with filters should alert us to any issues here. In addition, data plugin owners may be able to confirm that `value` is no longer non-persistable.

x-pack/plugins/lens/server/saved_objects.ts

drewdaemon · 2021-12-03T21:53:51Z

@flash1293 I am currently injecting the inject/extract functions from the data plugin's FilterManager class during the setup cycle. It occurs to me that publicly exporting these functions instead would enable us to import them statically. It's a simpler solution. Here is an illustration of what I mean.

What do you think about those options?

flash1293 · 2021-12-06T09:34:04Z

@andrewctate Agree, statically exporting these helpers seems like the easiest approach. Similar to how the search namespace is exported here maybe:

kibana/src/plugins/data/server/index.ts

Line 95 in 4385ac4

CidrMask,

@ppisljar @Dosant do you have any guidance here how it should work?

[suggestion] statically import filter inject/extract

…ts' of github.com:andrewctate/kibana into 114482/correctly-apply-filter-migrations-in-saved-objects

…y-apply-filter-migrations-in-saved-objects

flash1293

Tested again and works fine, thanks! I would appreciate another look just to make sure - as discussed these changes are really important to get right

x-pack/plugins/lens/server/migrations/common_migrations.ts

drewdaemon · 2022-01-10T14:08:17Z

@elasticmachine merge upstream

…saved-objects

drewdaemon · 2022-01-11T20:24:34Z

@alexwizp @stratoula Tim suggested one of you to take a look at the migrations in this PR. Would one of you be available to give it a review?

stratoula · 2022-01-12T13:16:33Z

@elasticmachine merge upstream

…saved-objects

stratoula

I tested various Lens charts with filter combinations created in v8 and migrated to v8.1. They seem to work fine.

I just have a question for the "Unsaved changes modal"

Let's say that I have an embedded lens chart. I dont change anything, I just select a different dataview from the left panel
For example here

I changed the index pattern dropdown from flights to ecommerce but I didn't make any change to the chart. Why do I see the modal?

I tested it in v8 and the modal doesn't appear (which makes more sense to me)

kibana-ci · 2022-01-12T15:03:18Z

💚 Build Succeeded

Metrics [docs]

Public APIs missing comments

Total count of every public API that lacks a comment. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats comments for more detailed information.

id	before	after	diff
`data`	2747	2748	+1
`lens`	246	248	+2
total			+3

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id	before	after	diff
`lens`	1023.3KB	1023.9KB	+599.0B
`osquery`	939.3KB	939.2KB	-14.0B
total			+585.0B

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id	before	after	diff
`lens`	41.0KB	41.1KB	+101.0B

Unknown metric groups

API count

id	before	after	diff
`data`	3341	3342	+1
`lens`	263	265	+2
total			+3

References to deprecated APIs

id	before	after	diff
`lens`	269	278	+9

History

💚 Build #16713 succeeded 7077464
💚 Build #16398 succeeded b60fae8
💚 Build #16013 succeeded e63a5d7
💚 Build #15966 succeeded 97494a7
💚 Build #15913 succeeded ba6172b
💚 Build #15877 succeeded 9bdd538

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

drewdaemon · 2022-01-13T00:24:20Z

@mbondyra good find. I will look into how difficult it would be to exclude that particular setting from the state comparison.

On the other hand, the selected data view is persisted in the saved object according to @flash1293 , so there's may also be argument for warning the user if they navigate away without saving. I'm not sure on what the user would expect.

stratoula · 2022-01-13T06:31:10Z

Do we? Even if it is not used at all? In my example above I just changed index pattern from the dropdown, the field list is updated but I haven't used any of them in my viz. I cant replicate it tbh. I checked the SO and there is no reference of the unused dataview. 🤔

flash1293 · 2022-01-13T10:28:12Z

@stratoula It's not used on the dashboard, but it's used in the editor to re-open the data view in the data panel the user had opened the last time. It's stored in the references as "current index pattern":

I don't feel strongly about changing that and just not saving this piece of the state (it doesn't seem super relevant in the first place), but I would like to move it out of this PR if possible.

stratoula · 2022-01-13T10:57:53Z

Yes you are right Joe. It is saved on the SO but it doesn't seem to be used anywhere. If I save a lens chart like that

and then re-open it, the dataview in the dropdown is the one used on the chart and not the one I selected on the dropdown.
I will create an issue for that, it doesn't seem necessary to keep this in the SO if it is not used anywhere.

@andrewctate no need to block this PR for this. I think it works fine 👍

drewdaemon · 2022-01-13T17:11:51Z

@tomsonpl thanks for the review!

@stratoula okay, I will merge as-is. Feel free to assign that issue to me!

drewdaemon added 7 commits December 1, 2021 18:12

inject filterManager.extract into selector

59144f5

tentative migration

49b2932

Replace local injectFilterReferences with FilterManager.inject

f54cf88

apply migration in embedded context

133f488

some fixes

fbb3835

expose query service in server data plugin setup

f17bb31

apply filter migrations to embedded lens visualizations

7218e4f

drewdaemon force-pushed the 114482/correctly-apply-filter-migrations-in-saved-objects branch from da9dc2a to 7218e4f Compare December 3, 2021 16:21

drewdaemon commented Dec 3, 2021

View reviewed changes

x-pack/plugins/lens/server/saved_objects.ts Outdated Show resolved Hide resolved

drewdaemon changed the title ~~114482/correctly apply filter migrations in saved objects~~ [Lens] correctly apply filter migrations in saved objects Dec 3, 2021

fix imports

842304c

drewdaemon changed the title ~~[Lens] correctly apply filter migrations in saved objects~~ [Lens] give data plugin control of filter extraction, injection, and migrations Dec 3, 2021

drewdaemon added 2 commits December 3, 2021 15:14

fix type export

9ad9b37

statically import filter inject/extract

4ba7359

drewdaemon and others added 4 commits December 6, 2021 09:26

Merge pull request #1 from andrewctate/inject-extract-static-imports

2e791e5

[suggestion] statically import filter inject/extract

replacing dynamic injection with static import

d1594a8

a little more cleanup

1e8d09e

apply filter migrations to lens migration map

9ff5fa9

drewdaemon force-pushed the 114482/correctly-apply-filter-migrations-in-saved-objects branch from 172956b to 9ff5fa9 Compare December 6, 2021 17:52

drewdaemon added 2 commits December 6, 2021 11:54

remove unused query service dep

ec82855

move a test

6ce57e3

drewdaemon force-pushed the 114482/correctly-apply-filter-migrations-in-saved-objects branch from e9adaed to 6ce57e3 Compare December 6, 2021 18:37

drewdaemon added 2 commits January 5, 2022 08:42

fix applying filter migration in lens embedded context

c24b7ac

Merge branch '114482/correctly-apply-filter-migrations-in-saved-objec…

16e11db

…ts' of github.com:andrewctate/kibana into 114482/correctly-apply-filter-migrations-in-saved-objects

drewdaemon requested a review from flash1293 January 5, 2022 14:43

Merge branch 'main' of github.com:elastic/kibana into 114482/correctl…

ba6172b

…y-apply-filter-migrations-in-saved-objects

flash1293 approved these changes Jan 5, 2022

View reviewed changes

drewdaemon added 2 commits January 5, 2022 11:44

re-enabling skipped app test

97494a7

test that filter migrations are applied to lens migration map

e63a5d7

drewdaemon commented Jan 6, 2022

View reviewed changes

x-pack/plugins/lens/server/migrations/common_migrations.ts Outdated Show resolved Hide resolved

kibanamachine and others added 2 commits January 10, 2022 09:08

Merge branch 'main' into 114482/correctly-apply-filter-migrations-in-…

b60fae8

…saved-objects

expect Filter array instead of individual Filter object

7077464

Merge branch 'main' into 114482/correctly-apply-filter-migrations-in-…

7df6319

…saved-objects

stratoula reviewed Jan 12, 2022

View reviewed changes

patrykkopycinski requested a review from tomsonpl January 13, 2022 14:43

tomsonpl approved these changes Jan 13, 2022

View reviewed changes

drewdaemon merged commit efc07ee into elastic:main Jan 13, 2022

kibanamachine added the backport:skip This commit does not require backporting label Jan 13, 2022

drewdaemon mentioned this pull request Jan 13, 2022

[Lens] use data plugin migrations for search sources in legacy visualizations #123005

Merged

1 task

ppisljar mentioned this pull request Jan 17, 2022

[data.search] correctly consume migration utilities #116098

Open

7 tasks

drewdaemon mentioned this pull request Feb 7, 2022

[Lens] "Save and return" doesn't work when Lens has filters #124821

Closed

stratoula mentioned this pull request Dec 13, 2022

[Unified Search] Supports complex filters with AND/OR relationships #143928

Merged

11 tasks

banderror mentioned this pull request Dec 13, 2022

[Security Solution] Rule filters: saved object migrations #147441

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Lens] give data plugin control of filter extraction, injection, and migrations #120305

[Lens] give data plugin control of filter extraction, injection, and migrations #120305

drewdaemon commented Dec 2, 2021 •

edited

Loading

drewdaemon commented Dec 3, 2021 •

edited

Loading

flash1293 commented Dec 6, 2021

flash1293 left a comment

drewdaemon commented Jan 10, 2022

drewdaemon commented Jan 11, 2022

stratoula commented Jan 12, 2022

stratoula left a comment

kibana-ci commented Jan 12, 2022

API count

References to deprecated APIs

drewdaemon commented Jan 13, 2022

stratoula commented Jan 13, 2022 •

edited

Loading

flash1293 commented Jan 13, 2022

stratoula commented Jan 13, 2022

drewdaemon commented Jan 13, 2022

[Lens] give data plugin control of filter extraction, injection, and migrations #120305

[Lens] give data plugin control of filter extraction, injection, and migrations #120305

Conversation

drewdaemon commented Dec 2, 2021 • edited Loading

Summary

How to Test

Saved Object Migrations

"Unsaved changes" Modal

Setup

Does NOT prompt if nothing has changed

DOES prompt if something has changed

Special case: datasource change

Checklist

Risk Matrix

drewdaemon commented Dec 3, 2021 • edited Loading

flash1293 commented Dec 6, 2021

flash1293 left a comment

Choose a reason for hiding this comment

drewdaemon commented Jan 10, 2022

drewdaemon commented Jan 11, 2022

stratoula commented Jan 12, 2022

stratoula left a comment

Choose a reason for hiding this comment

kibana-ci commented Jan 12, 2022

💚 Build Succeeded

Metrics [docs]

Public APIs missing comments

Async chunks

Page load bundle

API count

References to deprecated APIs

History

drewdaemon commented Jan 13, 2022

stratoula commented Jan 13, 2022 • edited Loading

flash1293 commented Jan 13, 2022

stratoula commented Jan 13, 2022

drewdaemon commented Jan 13, 2022

drewdaemon commented Dec 2, 2021 •

edited

Loading

drewdaemon commented Dec 3, 2021 •

edited

Loading

stratoula commented Jan 13, 2022 •

edited

Loading